Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft PR: Working on issue #7824 #7903

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Arup-Chauhan
Copy link

Working on issue #7824

Copy link

cla-checker-service bot commented Jul 22, 2024

💚 CLA has been signed

Copy link

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

@Arup-Chauhan
Copy link
Author

Arup-Chauhan commented Jul 22, 2024

Hi @cee-chen, I have made this PR

Will be starting with the direction @daniel-rhoades mentioned to use the makeHighContrastColor function in the code.

Would it be a good start?

@cee-chen
Copy link
Member

@Arup-Chauhan My preference would be to skip makeHighContrastColor and custom text color support. To be frank, it's not something I want to support in EuiBadge or EuiAvatar right now.

I think the simplest path forward would be to DRY out the shared color contrast logic between EuiAvatar and EuiBadge to an exported utility (which we can locate in src/services/). This will enable EuiAvatar to also throw a console warning if color contrast is insufficient.

@Arup-Chauhan
Copy link
Author

@Arup-Chauhan My preference would be to skip makeHighContrastColor and custom text color support. To be frank, it's not something I want to support in EuiBadge or EuiAvatar right now.

I think the simplest path forward would be to DRY out the shared color contrast logic between EuiAvatar and EuiBadge to an exported utility (which we can locate in src/services/). This will enable EuiAvatar to also throw a console warning if color contrast is insufficient.

so we are looking at a shared utility for the color contrast logic and place it in src/services/. This will eliminate duplicate implementations in EuiAvatar and EuiBadge. By using this utility, EuiAvatar can also throw a console warning if the color contrast is insufficient.

Is this a correct way?

@cee-chen
Copy link
Member

That's correct!

@Arup-Chauhan
Copy link
Author

That's correct!

Cool, thanks for the feedback, I will start doing it and let you know how it goes, have a good day!

@Arup-Chauhan
Copy link
Author

@cee-chen Hi, just wanted to update you that I am in the middle of some summer break commitments from college, will resume working on this soon asap.

Hope that works!

Copy link

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

@Arup-Chauhan
Copy link
Author

@cee-chen Hi Cee, wanted to share that I have kind of coded a logic using the constrst utility, will push the changes in this draft repo for your review by the end of this week.

Thanks for the patience, the college semester course load is a little tough this time so spending my major amount there!

@cee-chen
Copy link
Member

No worries at all. This isn't a high priority task, so take your time!

@Arup-Chauhan
Copy link
Author

No worries at all. This isn't a high priority task, so take your time!

Hi @cee-chen , I hope you're doing well. I noticed that the issue is marked as closed. I wasn’t able to update you earlier due to some urgent academic commitments. If it’s still possible, I would love to continue working on this.

I've made some progress on creating the shared color contrast utility to eliminate duplicate implementations in components like EuiAvatar and EuiBadge. Below is a snippet showing the approach for calculating the color contrast ratio:

/**
 * Calculates the contrast ratio between two colors.
 * @param {string} foreground - The foreground color in hexadecimal format.
 * @param {string} background - The background color in hexadecimal format.
 * @returns {number} - The contrast ratio between the two colors.
 */
function calculateContrast(foreground: string, background: string): number {
    const lum1 = getLuminance(foreground);
    const lum2 = getLuminance(background);
    const brightest = Math.max(lum1, lum2);
    const darkest = Math.min(lum1, lum2);
    return (brightest + 0.05) / (darkest + 0.05);
}

This utility will help ensure the colors meet accessibility standards. Let me know if I can proceed with further refinements or if there are any changes you'd like me to incorporate.

Looking forward to your response!

@cee-chen
Copy link
Member

cee-chen commented Oct 18, 2024

There isn't a need for a custom calculateContrast function - we already have this via our chroma.js library:

export const getColorContrast = (textColor: string, color: string) => {
return chroma.contrast(textColor, color);
};

You should be able to simply re-use the existing util and EuiBadge's logic to throw an error when color contrast is too low:

// Check the contrast ratio. If it's low contrast, emit a console awrning
const contrastRatio = getColorContrast(textColor, color);
if (contrastRatio < wcagContrastMin) {
console.warn(
`Warning: ${color} badge has a low contrast of ${contrastRatio.toFixed(
2
)}. Should be above ${wcagContrastMin}.`
);
}

My preferred approach would be moving the getColorContrast from EuiBadge's color utils to packages/eui/src/services/color/contrast.ts, and adding a new warnIfContrastBelowMin() util.

@Arup-Chauhan
Copy link
Author

Hi @cee-chen, thanks for being patient as work through on this issue and thanks for your feedback

I have made the following changes and pushed it in the draft PR too.

Replaced the existing contrast-checking logic in EuiBadge with calls to the centralized utility functions as suggested.

  1. Created the centralized getColorContrast and warnIfContrastBelowMin() utilities in packages/eui/src/services/color/contrast.ts:
import chroma from 'chroma-js';

export const getColorContrast = (textColor: string, backgroundColor: string): number => {
  return chroma.contrast(textColor, backgroundColor);
};

export const warnIfContrastBelowMin = (textColor: string, backgroundColor: string, wcagContrastMin: number): void => {
  const contrastRatio = getColorContrast(textColor, backgroundColor);
  if (contrastRatio < wcagContrastMin) {
    console.warn(
      `Warning: ${backgroundColor} background with ${textColor} text has a low contrast ratio of ${contrastRatio.toFixed(
        2
      )}. Should be above ${wcagContrastMin}.`
    );
  }
};
  1. Replaced the existing logic in EuiBadge:

Original code:

const contrastRatio = getColorContrast(textColor, color);
if (contrastRatio < wcagContrastMin) {
  console.warn(
    `Warning: ${color} badge has a low contrast of ${contrastRatio.toFixed(
      2
    )}. Should be above ${wcagContrastMin}.`
  );
}

Replaced with:

import { warnIfContrastBelowMin } from '../../../services/color/contrast';

warnIfContrastBelowMin(textColor, color, wcagContrastMin);

This change centralizes the contrast-checking logic, leveraging chroma.js as recommended for improved consistency and maintainability. Please let me know if there are any further adjustments or feedback!

Also, I would like to know how to check the changes and verify them

@Arup-Chauhan Arup-Chauhan reopened this Nov 9, 2024
Copy link

github-actions bot commented Nov 9, 2024

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants